feat: add .env file support for provider credentials#26
Conversation
- Add dotenvy dependency for .env file parsing - Search .env from current dir up to 3 parent directories (monorepo-friendly) - Add --no-env global flag to skip .env loading - Credentials priority: .env < credentials.toml < keychain - Support env vars: OPENROUTER_API_KEY, FALAI_API_KEY, REPLICATE_API_TOKEN, WAVESPEED_API_KEY
📝 WalkthroughWalkthroughAdds optional .env discovery/loading and extraction of provider credentials from environment variables, a global Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Parser
participant Main as main()
participant Dotenv as load_dotenv()
participant Config as config::load_config_with_env()
participant Env as Environment
participant Keychain as Keychain
participant File as credentials.toml
CLI->>Main: Parse args (including --no-env)
Main->>Dotenv: call load_dotenv() if load_env == true
Dotenv->>Env: search cwd & parents for .env
Dotenv-->>Main: return .env path or None
Main->>Config: call load_config_with_env(load_env)
Config->>Env: credentials_from_env()
Env-->>Config: provider credential map
Config->>Keychain: load keychain credentials
Keychain-->>Config: keychain credential map
Config->>File: load credentials.toml
File-->>Config: file credential map
Config->>Config: merge maps (file overrides env/keychain)
Config-->>Main: return AppConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds .env-based credential loading to the CLI so provider credentials can be supplied via environment variables (optionally discovered from .env files in the current directory or up to 3 parent directories), alongside existing credentials.toml/keychain support.
Changes:
- Load a
.envfile from CWD / parent dirs on startup (unless--no-envis set). - Merge provider credentials from environment variables into
AppConfigduring config load. - Add
dotenvydependency and unit tests for env extraction +.envdiscovery.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Loads .env at startup, gated by new --no-env flag. |
| src/config/mod.rs | Adds .env search/loading + env-var credential extraction, and introduces load_config_with_env. |
| src/cli/mod.rs | Adds global --no-env CLI flag. |
| Cargo.toml | Adds dotenvy dependency. |
| Cargo.lock | Locks dotenvy and updates dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[test] | ||
| fn test_credentials_from_env_empty() { | ||
| std::env::remove_var("OPENROUTER_API_KEY"); | ||
| std::env::remove_var("FALAI_API_KEY"); | ||
| std::env::remove_var("REPLICATE_API_TOKEN"); | ||
| std::env::remove_var("WAVESPEED_API_KEY"); | ||
|
|
||
| let creds = credentials_from_env(); | ||
| assert!(creds.is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_credentials_from_env_with_values() { | ||
| std::env::set_var("OPENROUTER_API_KEY", "test-openrouter-key"); | ||
| std::env::set_var("FALAI_API_KEY", "test-falai-key"); | ||
|
|
||
| let creds = credentials_from_env(); | ||
| assert_eq!( | ||
| creds.get("openrouter").and_then(|c| c.get("api_key")), | ||
| Some(&"test-openrouter-key".to_string()) | ||
| ); | ||
| assert_eq!( | ||
| creds.get("falai").and_then(|c| c.get("api_key")), | ||
| Some(&"test-falai-key".to_string()) | ||
| ); | ||
|
|
||
| std::env::remove_var("OPENROUTER_API_KEY"); | ||
| std::env::remove_var("FALAI_API_KEY"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_credentials_from_env_ignores_empty() { | ||
| std::env::set_var("OPENROUTER_API_KEY", ""); | ||
|
|
||
| let creds = credentials_from_env(); | ||
| assert!(!creds.contains_key("openrouter")); | ||
|
|
||
| std::env::remove_var("OPENROUTER_API_KEY"); | ||
| } |
There was a problem hiding this comment.
These tests mutate process-global environment variables via std::env::set_var/remove_var without restoring prior values. Because Rust tests run in parallel by default, this can lead to flaky failures and can clobber state for other tests in the same process. Capture previous values and restore them (RAII guard), and/or serialize env-mutating tests behind a global mutex.
| #[test] | ||
| fn test_load_dotenv_finds_file_in_current_dir() { | ||
| use std::io::Write; | ||
| let temp_dir = tempfile::tempdir().unwrap(); | ||
| let env_path = temp_dir.path().join(".env"); | ||
| let mut file = std::fs::File::create(&env_path).unwrap(); | ||
| writeln!(file, "TEST_VAR=test_value").unwrap(); | ||
|
|
||
| let original_cwd = std::env::current_dir().unwrap(); | ||
| std::env::set_current_dir(temp_dir.path()).unwrap(); | ||
|
|
||
| std::env::remove_var("TEST_VAR"); | ||
| let loaded_path = load_dotenv(); | ||
|
|
||
| assert!(loaded_path.is_some()); | ||
| assert_eq!( | ||
| std::env::var("TEST_VAR").ok(), | ||
| Some("test_value".to_string()) | ||
| ); | ||
|
|
||
| std::env::set_current_dir(original_cwd).unwrap(); | ||
| std::env::remove_var("TEST_VAR"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_load_dotenv_searches_parent_dirs() { | ||
| use std::io::Write; | ||
| let temp_dir = tempfile::tempdir().unwrap(); | ||
| let env_path = temp_dir.path().join(".env"); | ||
| let mut file = std::fs::File::create(&env_path).unwrap(); | ||
| writeln!(file, "PARENT_TEST_VAR=parent_value").unwrap(); | ||
|
|
||
| let child_dir = temp_dir.path().join("child"); | ||
| std::fs::create_dir(&child_dir).unwrap(); | ||
|
|
||
| let original_cwd = std::env::current_dir().unwrap(); | ||
| std::env::set_current_dir(&child_dir).unwrap(); | ||
|
|
||
| std::env::remove_var("PARENT_TEST_VAR"); | ||
| let loaded_path = load_dotenv(); | ||
|
|
||
| assert!(loaded_path.is_some()); | ||
| assert_eq!( | ||
| std::env::var("PARENT_TEST_VAR").ok(), | ||
| Some("parent_value".to_string()) | ||
| ); | ||
|
|
||
| std::env::set_current_dir(original_cwd).unwrap(); | ||
| std::env::remove_var("PARENT_TEST_VAR"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_load_dotenv_returns_none_when_no_file() { | ||
| let temp_dir = tempfile::tempdir().unwrap(); | ||
|
|
||
| let original_cwd = std::env::current_dir().unwrap(); | ||
| std::env::set_current_dir(temp_dir.path()).unwrap(); | ||
|
|
||
| let loaded_path = load_dotenv(); | ||
| assert!(loaded_path.is_none()); | ||
|
|
||
| std::env::set_current_dir(original_cwd).unwrap(); | ||
| } |
There was a problem hiding this comment.
The load_dotenv tests change the process working directory via set_current_dir, which is also process-global and not safe with parallel test execution. This can make unrelated tests flaky if they assume a stable CWD. Consider serializing the CWD-mutating tests behind a global mutex and restoring the original directory in a drop guard so it’s reset even if the test panics.
src/main.rs
Outdated
| if !cli.no_env { | ||
| if let Some(env_path) = config::load_dotenv() { | ||
| tracing::info!("Loaded .env from: {:?}", env_path); | ||
| } | ||
| } |
There was a problem hiding this comment.
--no-env currently only skips load_dotenv() in main, but config loading still reads provider credentials from environment variables because downstream code calls config::load_config() (which hard-codes load_config_with_env(true)). This makes --no-env ineffective for the documented behavior (“use credentials manager only”). Consider threading the flag through so CLI handlers load config via load_config_with_env(!cli.no_env) (or load config once in run and pass it down).
| // Load credentials from environment variables first (lowest priority). | ||
| if load_env { | ||
| let env_creds = credentials_from_env(); | ||
| for (provider_id, creds) in env_creds { | ||
| let provider_config = config.providers.entry(provider_id).or_default(); | ||
| for (key, value) in creds { | ||
| provider_config.credentials.entry(key).or_insert(value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Env credentials are inserted with or_insert here, but later in load_config_with_env the credentials.toml merge also uses or_insert (see further down in this function). That combination makes env credentials higher priority than credentials.toml, which contradicts the documented precedence .env (lowest) → credentials.toml → keychain. Adjust the merge logic so values from credentials.toml can override env-provided values while still preserving keychain as the highest priority.
| pub fn load_config() -> Result<AppConfig, InfsError> { | ||
| load_config_with_env(true) | ||
| } | ||
|
|
||
| pub fn load_config_with_env(load_env: bool) -> Result<AppConfig, InfsError> { | ||
| let config_path = get_config_path()?; | ||
|
|
||
| let mut config = if config_path.exists() { |
There was a problem hiding this comment.
load_config() now always calls load_config_with_env(true), so there’s no way for the CLI’s --no-env flag to prevent env-var credential extraction unless callers are updated to use load_config_with_env(false). If --no-env is meant to disable all env-based credentials (not just .env file parsing), consider making load_config() accept an option or ensuring all CLI call sites switch to load_config_with_env(!no_env).
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/mod.rs`:
- Around line 215-223: The code currently loads .env creds first and then uses
or_insert when applying credentials.toml, preventing file values from overriding
env. To make credentials.toml higher priority than .env, change the insertion
used when processing the credentials.toml block (the code that currently calls
provider_config.credentials.entry(key).or_insert(value) around the
credentials.toml loading) to perform an unconditional insert/overwrite (e.g.,
provider_config.credentials.insert(key, value)) or load the file before env;
keep the env-loading block (credentials_from_env()) using or_insert so env only
supplies defaults.
- Around line 533-635: The tests mutate global process env and CWD causing
races; update the tests (test_credentials_from_env_empty,
test_credentials_from_env_with_values, test_credentials_from_env_ignores_empty,
test_load_dotenv_finds_file_in_current_dir,
test_load_dotenv_searches_parent_dirs,
test_load_dotenv_returns_none_when_no_file) to run serially and to always
restore state: add a global synchronization (e.g., a static Mutex via
lazy_static or once_cell) and acquire it at the start of each test, and wrap
env/CWD changes in a guard that restores the original current_dir and original
env vars in a Drop implementation (or use the serial_test crate with #[serial]
plus explicit restore) so load_dotenv() and credentials_from_env() calls no
longer race with parallel tests.
In `@src/main.rs`:
- Around line 26-30: The --no-env flag currently only skips
config::load_dotenv() but downstream still calls load_config_with_env(true), so
environment variables are still used; propagate the CLI flag by computing a
load_env boolean (e.g., let load_env = !cli.no_env) and pass that into
load_config_with_env(load_env) wherever config loading occurs (replace hardcoded
true), and also use load_env to conditionally call config::load_dotenv()
(instead of checking !cli.no_env in two places) so all env ingestion paths
(load_dotenv() and load_config_with_env) honor the same flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2b72372-ccdf-4bcb-9cee-8c6f669854be
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/cli/mod.rssrc/config/mod.rssrc/main.rs
src/main.rs
Outdated
| if !cli.no_env { | ||
| if let Some(env_path) = config::load_dotenv() { | ||
| tracing::info!("Loaded .env from: {:?}", env_path); | ||
| } | ||
| } |
There was a problem hiding this comment.
--no-env currently disables .env file loading only, not env credential ingestion.
At Line 26, you gate load_dotenv(), but downstream config loading still defaults to load_config_with_env(true) (see src/config/mod.rs Line 199). So --no-env does not enforce “credentials manager only” if process env vars are set.
[suggested fix] Thread !cli.no_env into config loading paths and use load_config_with_env(load_env) consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 26 - 30, The --no-env flag currently only skips
config::load_dotenv() but downstream still calls load_config_with_env(true), so
environment variables are still used; propagate the CLI flag by computing a
load_env boolean (e.g., let load_env = !cli.no_env) and pass that into
load_config_with_env(load_env) wherever config loading occurs (replace hardcoded
true), and also use load_env to conditionally call config::load_dotenv()
(instead of checking !cli.no_env in two places) so all env ingestion paths
(load_dotenv() and load_config_with_env) honor the same flag.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/config/mod.rs (2)
241-260:⚠️ Potential issue | 🟠 MajorKeychain still loses to
credentials.toml.This block hydrates keychain first, then
merge_file_credentials()overwrites any matching key. That makescredentials.tomlthe highest-priority source, not the documented.env→credentials.toml→ keychain order. A regression test for file-vs-keychain precedence would be worth adding with the fix.💡 Suggested fix
- // Load credentials: keychain next (for keys recorded in keychain_credentials), - // then fall back to credentials.toml for anything not yet migrated. - for (provider_id, provider_config) in config.providers.iter_mut() { - for cred_key in &provider_config.keychain_credentials { - if let Some(value) = keyring_get(provider_id, cred_key)? { - provider_config.credentials.insert(cred_key.clone(), value); - } - } - } - // Merge credentials from separate file (even if config.toml was absent or for // providers whose keys are not yet in keychain_credentials). let creds_path = get_credentials_path()?; if creds_path.exists() { let creds_content = std::fs::read_to_string(&creds_path) @@ let creds: HashMap<String, ProviderConfig> = toml::from_str(&creds_content) .map_err(|e| InfsError::ConfigError(format!("Failed to parse credentials: {}", e)))?; merge_file_credentials(&mut config, creds); } + + // Load keychain last (highest priority). + for (provider_id, provider_config) in config.providers.iter_mut() { + for cred_key in &provider_config.keychain_credentials { + if let Some(value) = keyring_get(provider_id, cred_key)? { + provider_config.credentials.insert(cred_key.clone(), value); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/mod.rs` around lines 241 - 260, The current code hydrates keychain into provider_config.credentials but then calls merge_file_credentials which overwrites those values, making credentials.toml take precedence; fix by ensuring keychain wins: either call merge_file_credentials before the keyring loop or modify merge_file_credentials to only insert keys that do not already exist in config.providers[*].credentials (i.e., preserve existing entries filled from keychain), referencing merge_file_credentials, keychain_get, provider_config.keychain_credentials, provider_config.credentials and get_credentials_path; add a regression test asserting that a key present in the keychain is not replaced by credentials.toml.
111-120:⚠️ Potential issue | 🟠 MajorEnv-backed providers never become "connected".
merge_env_credentials()fills a freshProviderConfigbut leavesconnectedatfalse. Downstream status checks still key offconnected/get_api_key(), so providers discovered only via.envkeep showing up as disconnected. Prefer deriving status from the effective config instead of persisted flags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/mod.rs` around lines 111 - 120, merge_env_credentials currently populates a new ProviderConfig but leaves its connected flag false so env-only providers appear disconnected; update merge_env_credentials (and/or ProviderConfig initialization logic) so after inserting creds into provider_config.credentials you derive and set provider_config.connected based on the effective config (e.g., set connected = true if provider_config.get_api_key().is_some() or other required fields are present) instead of relying on a persisted flag — ensure you reference provider_config.credentials and provider_config.connected (or the get_api_key method) when computing the status so env-discovered providers report as connected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/mod.rs`:
- Around line 220-222: The current load_config() unconditionally calls
load_config_with_env(true), which causes callers (including provider
connect/disconnect paths that write persistent storage) to ingest and then
persist env-only secrets; change load_config() to call
load_config_with_env(false) so environment sources are not loaded by default,
and ensure only explicit callers that should accept env secrets call
load_config_with_env(true) (e.g., interactive or import flows), leaving provider
connect/disconnect to use the non-env default to avoid persisting env-only
secrets.
---
Duplicate comments:
In `@src/config/mod.rs`:
- Around line 241-260: The current code hydrates keychain into
provider_config.credentials but then calls merge_file_credentials which
overwrites those values, making credentials.toml take precedence; fix by
ensuring keychain wins: either call merge_file_credentials before the keyring
loop or modify merge_file_credentials to only insert keys that do not already
exist in config.providers[*].credentials (i.e., preserve existing entries filled
from keychain), referencing merge_file_credentials, keychain_get,
provider_config.keychain_credentials, provider_config.credentials and
get_credentials_path; add a regression test asserting that a key present in the
keychain is not replaced by credentials.toml.
- Around line 111-120: merge_env_credentials currently populates a new
ProviderConfig but leaves its connected flag false so env-only providers appear
disconnected; update merge_env_credentials (and/or ProviderConfig initialization
logic) so after inserting creds into provider_config.credentials you derive and
set provider_config.connected based on the effective config (e.g., set connected
= true if provider_config.get_api_key().is_some() or other required fields are
present) instead of relying on a persisted flag — ensure you reference
provider_config.credentials and provider_config.connected (or the get_api_key
method) when computing the status so env-discovered providers report as
connected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0410ef22-2b6a-40b6-9c5e-90f66893910b
📒 Files selected for processing (5)
src/cli/app.rssrc/cli/doctor.rssrc/cli/provider.rssrc/config/mod.rssrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/mod.rs (1)
241-261:⚠️ Potential issue | 🟠 MajorCredential precedence is still inverted:
credentials.tomloverrides keychain.The PR objective states precedence as:
.env→credentials.toml→ keychain (lowest → highest). However, the current code loads keychain credentials (Lines 243-249) before file credentials (Lines 253-260), andmerge_file_credentialsuses unconditionalinsert(). This meanscredentials.tomlvalues overwrite keychain values, making keychain the lower priority source.To match the stated precedence, swap the order so keychain loading happens last:
Proposed fix to correct precedence
// Load credentials from environment variables first (lowest priority). if load_env { merge_env_credentials(&mut config, credentials_from_env()); } - // Load credentials: keychain next (for keys recorded in keychain_credentials), - // then fall back to credentials.toml for anything not yet migrated. - for (provider_id, provider_config) in config.providers.iter_mut() { - for cred_key in &provider_config.keychain_credentials { - if let Some(value) = keyring_get(provider_id, cred_key)? { - provider_config.credentials.insert(cred_key.clone(), value); - } - } - } - - // Merge credentials from separate file (even if config.toml was absent or for - // providers whose keys are not yet in keychain_credentials). + // Merge credentials from separate file next (overrides env). let creds_path = get_credentials_path()?; if creds_path.exists() { let creds_content = std::fs::read_to_string(&creds_path) .map_err(|e| InfsError::ConfigError(format!("Failed to read credentials: {}", e)))?; let creds: HashMap<String, ProviderConfig> = toml::from_str(&creds_content) .map_err(|e| InfsError::ConfigError(format!("Failed to parse credentials: {}", e)))?; merge_file_credentials(&mut config, creds); } + + // Load credentials from keychain last (highest priority). + for (provider_id, provider_config) in config.providers.iter_mut() { + for cred_key in &provider_config.keychain_credentials { + if let Some(value) = keyring_get(provider_id, cred_key)? { + provider_config.credentials.insert(cred_key.clone(), value); + } + } + } Ok(config) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/mod.rs` around lines 241 - 261, The current load order makes credentials.toml override keychain; to fix, load file credentials (get_credentials_path, reading/parsing and calling merge_file_credentials) before applying keychain entries, then apply keychain values last so they take precedence; update merge_file_credentials (or its usage) to not overwrite existing keys if you keep file-then-keychain approach, or simply ensure the loop that uses keyring_get and provider_config.credentials.insert runs after merge_file_credentials so keyring values replace any file values (refer to provider_config.keychain_credentials, provider_config.credentials.insert, keyring_get, get_credentials_path, merge_file_credentials).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/config/mod.rs`:
- Around line 241-261: The current load order makes credentials.toml override
keychain; to fix, load file credentials (get_credentials_path, reading/parsing
and calling merge_file_credentials) before applying keychain entries, then apply
keychain values last so they take precedence; update merge_file_credentials (or
its usage) to not overwrite existing keys if you keep file-then-keychain
approach, or simply ensure the loop that uses keyring_get and
provider_config.credentials.insert runs after merge_file_credentials so keyring
values replace any file values (refer to provider_config.keychain_credentials,
provider_config.credentials.insert, keyring_get, get_credentials_path,
merge_file_credentials).
Summary
.envfiles from current directory and up to 3 parent directories (monorepo-friendly)--no-envglobal flag to skip.envloading and use credentials manager only.env→credentials.toml→ keychainSupported environment variables
OPENROUTER_API_KEYFALAI_API_KEYREPLICATE_API_TOKENWAVESPEED_API_KEYUsage
Tests
.envloading (current dir, parent dirs, no file)Summary by CodeRabbit